Skip to content

Fix TestCase::expectErrorLog() with open_basedir php.ini - #6208

Closed
mvorisek wants to merge 3 commits into
sebastianbergmann:12.1from
mvorisek:fix_6197
Closed

Fix TestCase::expectErrorLog() with open_basedir php.ini#6208
mvorisek wants to merge 3 commits into
sebastianbergmann:12.1from
mvorisek:fix_6197

Conversation

@mvorisek

Copy link
Copy Markdown
Contributor

fix #6197

Comment thread src/Framework/TestCase.php
Comment thread tests/end-to-end/generic/expect-error-log-fail-with-open_basedir.phpt Outdated
* - https://github.com/php/php-src/issues/17817
* - https://github.com/php/php-src/issues/18530
*
* Until then, ignore TestCase::expectErrorLog() if open_basedir php.ini is in effect.

@staabm staabm May 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignoring expectations sounds like a bad idea.. maybe we should turn such a test "risky" and/or can introduce a warning telling the author of the test, that not everything worked according to plan.

siltent ignore sounds like a bad idea.

I guess @sebastianbergmann has an opinion on that one

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

"incomplete" should be the right status. If error_log file could not be created, test would still complete but will be marked as incomplete.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the "throw incomplete exception" be moved then into TestCase::runBare to always run https://github.com/sebastianbergmann/phpunit/blob/12.1.5/src/Framework/TestCase.php#L500-L502?

Another option is to revert/remove the newly introduced TestCase::expectErrorLog() until php-src allows to check it without temporary file. Currently, it suffers not only the issue here, but it is also not the fastest to having to create temporary file before each TestCase run.

@staabm staabm May 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it is also not the fastest to having to create temporary file before each TestCase run.

please share a benchmark which proves your claim - noone else reported something like that since the change was released. how big is the impact?

@mvorisek mvorisek May 12, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<?php

$t1 = 0;
$t2 = 0;

for ($i = 0; $i < 1000; ++$i) {
    $ts = microtime(true);
    $capture = tmpfile();
    $ts2 = microtime(true);
    $t1 += $ts2 - $ts;
    $path = stream_get_meta_data($capture)['uri'];
    $t2 += microtime(true) - $ts2;
}

var_dump($t1 / $i);
var_dump($t2 / $i);

T2 does not call any FS, T1 is about 0.4 ms / per iteration (TestCase run).

@staabm staabm May 12, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean a real world impact, not microbenchmarks.
I am aware that not running stuff is faster then running stuff, but thats not helpful.

@mvorisek mvorisek May 12, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In another words about 1 second extra time per 2500 tests :)

This is the real world overhead of requiring FS.

Let @sebastianbergmann decide this thread incl. what to throw.

@mvorisek
mvorisek marked this pull request as draft May 12, 2025 08:22
@mvorisek
mvorisek marked this pull request as ready for review May 12, 2025 09:09
@sebastianbergmann

Copy link
Copy Markdown
Owner

I hope to find the time to look into this within the next week.

@sebastianbergmann
sebastianbergmann deleted the branch sebastianbergmann:12.1 June 6, 2025 02:59
@mvorisek

mvorisek commented Jun 6, 2025

Copy link
Copy Markdown
Contributor Author

@sebastianbergmann it seems you deleted the target brach without retargetting this PR first. Please have a look on this PR, currently we cannot upgrade to PHPUnit 12 everywhere because of this issue.

@sebastianbergmann

Copy link
Copy Markdown
Owner

I am so sorry about that! I need to add branch retargeting to my list of things to consider when releasing a new minor version and will try my best to avoid a situation like this in the future.

That being said, I cannot reopen this pull request as the target branch no longer exists. Therefore, I cannot change the target of this pull request to 12.2.

Can you please send a new pull request against 12.2 (or advise me on how to reopen/retarget)? Thank you!

@mvorisek

mvorisek commented Jun 6, 2025

Copy link
Copy Markdown
Contributor Author

👍 I cannot tell if the target branch will be recreated, if the PR could be reopened then or not. I will create another one into 12.2.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 14, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants